Payouts: Show bank reference key on payout details page #9886
Conversation
nagpai
requested review from
aheckler,
rogermattic,
souravdebnath1986 and
a team
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +956 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
A thought: if a merchant's looking for a bank reference key for a particular payout, it's probably because they think that their payout is missing. They almost certainly plan on passing the reference key along to their bank so that their bank can trace it. To help them with that, we could make a few adjustments:
On the Payments > Payouts page, when the key is not available, we show |
|
@aheckler - Those are great points! I have removed the |
|
Thanks for the great feedback points, @aheckler
I checked and these characters look distinct. Here is a screenshot. I hope it is fine
I have implemented these via latest commits. These were very useful points, thanks! |
|
Hwy @nagpai ! Thanks for working on this. I agree with @aheckler's suggestions and am glad we’re adding such useful details to the payout page! I think it might be worth exploring if there’s a different way to arrange the details, as the heading section could potentially be improved. If that’s alright, I’ll take a look at it next week and get back to you.
|
| @@ -70,7 +70,7 @@ interface SummaryItemProps { | |||
| label: string; | |||
| value: string | JSX.Element; | |||
| valueClass?: string | false; | |||
| detail?: string; | |||
| detail?: string | JSX.Element; | |||
There was a problem hiding this comment.
Does this align with the types for the component we are overriding (SummaryNumber from Woo core?
There was a problem hiding this comment.
The original SummaryItemNumber ( child within the summary list ) , does not have a detail prop. It was introduced in the customized SummaryListItem for the Deposit Overview header.
There was a problem hiding this comment.
Unrelated to this PR, but I think we should avoid inheriting/overriding components like this. We risk depending on implementation details, so our plugin is sensitive to changes in Woo core.
Ideally we'd either:
- Use components from Woo/upstream as is, with no customisations or overrides.
- Implement our own self-contained components, with no dependencies.
There was a problem hiding this comment.
I haven't tested this yet – just a code review.
My main feedback is that the approach to DepositDateItem is convoluted, and I think we should be able to achieve what we need by making simple, self-contained components for each logical item in the header – either as SummaryItem, or a custom component similar to SummaryItem. Note also that SummaryItem (IMO) is not adding much value, it's just a wrapper for Woo core SummaryNumber; maybe we don't need it.
Related, the click-to-copy behaviour is nice, but not essential. If we can implement this without side effects, great; but if we have to use side effects it might not be worth it – high complexity for low benefit.
Some minor thoughts about the UI:
Bank reference keyis long, and there's not much space to play with here (especially if keys are long). Is there a more compact label that would be just as clear to merchants?N/Ais loud in all caps. I'm not sure what we do elsewhere, but I'd lean towardn/aor just rendernot availablesince it is similar size to a real key, and immediately clear.
client/deposits/details/index.tsx
Outdated
| copyButton.classList.remove( 'state--copied' ); | ||
| }, 2000 ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Can we implement the copy button behaviour in a more React-idiomatic way, without using a side effect?
There was a problem hiding this comment.
I have changed this and used useEffect to track the state of the copy button click. Thanks for the guidance to align with the declarative way of doing things in React!
client/deposits/details/index.tsx
Outdated
| </> | ||
| ) : ( | ||
| 'N/A' | ||
| ) } |
There was a problem hiding this comment.
Can we make a custom component that encapsulates all this behaviour, e.g. PayoutBankDetails?
That component would take care of:
- Rendering the bank account / payout destination (as before)
- Rendering the bank ref, with a clickable button to copy
- Handling the click on the bank ref & copying to clipboard
- Setting state somewhere to show that the ref has been copied
- Whatever is needed to look / behave like a
SummaryNumber
Then, we could just render that component alongside all the others in the table header:
<SummaryList>
<SummaryItem … />
<SummaryItem … />
<PayoutBankDetails />
</SummaryList>There was a problem hiding this comment.
As per discussion during our pair coding session:
- If the component were to be built from scratch, it would be better to use a
Cardand add the design we need directly as a custom component here. The design requirement diverts quite a bit from theSummaryList+SummaryNumber/SummaryItem. - We will need to also check for behavior needed for conditional views like
Instant DepositandWithdrawalsif we redesign - For the current PR the next best thing we could do is to move
DepositDateItemas an independent component outsideDepositOverviewwhere it is currently nested in a complex way. This will make it simple, modular and more readable.
There was a problem hiding this comment.
@haszari - We discussed adding a smooth transition during our pair coding session. We are currently using SVG as mask-image attribute on the same button. This attribute is not smooth animatable from what i checked. It is classified as discrete .
I checked a workaround of rendering separate icons and making them appear and disappear with transitions, but not sure if the added complexity was worth it. Hence have not indulged in the transition animation for now.
client/deposits/details/style.scss
Outdated
| color: $dark-gray-500; | ||
|
|
||
| @include breakpoint( '<480px' ) { | ||
| word-wrap: break-word; |
There was a problem hiding this comment.
Is this useful at all screen sizes?
There was a problem hiding this comment.
Yup it is. Thanks for catching that. I will remove the media query.
| line-height: 36px; | ||
| text-align: left; | ||
| border-top: 1px solid $studio-gray-5; | ||
| } |
There was a problem hiding this comment.
What are we tweaking for smaller (<660px) screens? Should we make that behaviour the default, and progressively enhance on larger screens (i.e. mobile first)?
There was a problem hiding this comment.
We are making the font size smaller, making it left aligned and adding a border on top.
The layout by itself - overall is desktop first. The columns have a sideways scroll. My gut feel is that merchants would mostly use a tablet or a laptop to check WP admin. Hence would prefer keeping it as-is, but open to revisiting that! The padding property within the media query is redundant though, removed it - 8bda02c.
There was a problem hiding this comment.
We are making the font size smaller, making it left aligned and adding a border on top.
Why? Ideally the style changes for smaller screens are minimal (so it's easy to maintain), this sounds like a lot! If you are adding different behaviour for different screen sizes I'd recommend including details in the PR description – I see you have a screenshot, but even better if you explain why the changes are needed.
There was a problem hiding this comment.
If you are adding different behaviour for different screen sizes I'd recommend including details in the PR description – I see you have a screenshot, but even better if you explain why the changes are needed.
I have added a note on why we are doing the CSS change, in the PR description
The PR also adds a few CSS fixes for mobile view. This is needed to change the alignment of the summary items as column, and adjust borders + text alignment for smaller screens < 660px .
|
@haszari - Related to the UI feedback:
Just like for bank account / CC we show the last 4 digits, If we could ascertain the minimum number of characters sufficient to identify a payout at least at first glance. We could give the complete number as a tooltip or via copy.
I actually picked it up from the reports available within WooCommerce core 
|
That's an interesting idea! Though would complicate the UI a little. To be clear though, I was referring to the label not the value. The label |
Great - nice to see the consistency 🙌 |
|
@rogermattic has come back with some great design suggestions in the original issue linked to this PR. I am keeping the PR as |
There was a problem hiding this comment.
@rogermattic has come back with some #9878 (comment) in the original issue linked to this PR. I am keeping the PR as In progress for now, since next steps would depend on what design changes we adopt.
I think the new design is different enough it's worth closing this and starting a clean PR. If there is useful code in this PR, you can easily copy it over.
| @@ -70,7 +70,7 @@ interface SummaryItemProps { | |||
| label: string; | |||
| value: string | JSX.Element; | |||
| valueClass?: string | false; | |||
| detail?: string; | |||
| detail?: string | JSX.Element; | |||
There was a problem hiding this comment.
Unrelated to this PR, but I think we should avoid inheriting/overriding components like this. We risk depending on implementation details, so our plugin is sensitive to changes in Woo core.
Ideally we'd either:
- Use components from Woo/upstream as is, with no customisations or overrides.
- Implement our own self-contained components, with no dependencies.
client/deposits/details/index.tsx
Outdated
|
|
||
| if ( ! isCopied ) { | ||
| setIsCopied( true ); | ||
| setTimeout( () => { |
There was a problem hiding this comment.
Need to take care with setTimeout in React. In particular, need to clean up the timeout on unmount. (I need a refresher on this myself).
This article looks reasonable, there may be better info out there (especially on React official docs): https://felixgerschau.com/react-hooks-settimeout/
There was a problem hiding this comment.
Very helpful and a great concept I learned today! I have implemented it in the CopyButton component. I hope I have done it correctly. Would appreciate if you could check once 🙏🏼
client/deposits/details/index.tsx
Outdated
| onClick={ () => { | ||
| const bankReferenceId = document.querySelector( | ||
| '.woopayments-payout-bank-reference-id' | ||
| )?.textContent; |
There was a problem hiding this comment.
How can we get the value without fishing around in the DOM (clue: deposit prop 😁)? We try to avoid that in React; the component should not depend on the DOM, it should be self contained.
There was a problem hiding this comment.
Awesome hint! I wonder how I missed it 😄
Tweaked it here - af176bd
client/deposits/details/style.scss
Outdated
| &.state--copied i { | ||
| mask-image: url( 'assets/images/icons/check-green.svg?asset' ); | ||
| background-color: $studio-green-50; | ||
| } |
There was a problem hiding this comment.
There's a lot of css (and some React behaviour) for this little copy-to-clipboard button. Could we package it into a little component? The button is nicely general-purpose so is a good candidate for an abstraction.
|
I am closing this PR in favor of #9945 . I am working afresh on the new design recommendations to show bank reference ID in an uncluttered way. I have retained some useful work like the copy button component on the new PR. |
Fixes #9878
Changes proposed in this Pull Request
Note that in test payouts, the bank reference key is of 41 characters length. In real payouts, we see a variety of lengths, sometimes much smaller ( 12 characters ).
There is a copy button added too for easy copying by merchant.
The bank reference key shows as
N/Aif it is not available from Stripe.Testing instructions
acct_1OuYn1QpUShfetUA, if you like.npm startto build assets.Payments > PayoutsCopybutton beside the Bank reference key, and see that it copies the key to clipboard.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge